Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated dbmigrate to obtain source db type from config.toml #1258

Merged
merged 11 commits into from
Dec 14, 2022

Conversation

Taztingo
Copy link
Contributor

@Taztingo Taztingo commented Dec 9, 2022

Description

It was hard for dbmigrate to differentiate between goleveldb and cleveldb. I updated the code to take the db source type from the config.toml instead of attempting to detect it.

closes: #1257


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

Matthew Witkowski added 3 commits December 9, 2022 16:27
…from the config.toml. This was done because it was hard to distinguish between goleveldb and cleveldb.
@codecov
Copy link

codecov bot commented Dec 12, 2022

Codecov Report

Merging #1258 (5e19c8e) into main (d0c1fd6) will decrease coverage by 0.00%.
The diff coverage is 37.50%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1258      +/-   ##
==========================================
- Coverage   58.46%   58.46%   -0.01%     
==========================================
  Files         206      206              
  Lines       25258    25264       +6     
==========================================
+ Hits        14768    14771       +3     
- Misses       9395     9398       +3     
  Partials     1095     1095              
Impacted Files Coverage Δ
cmd/dbmigrate/utils/migrator.go 36.70% <37.50%> (+0.12%) ⬆️

Matthew Witkowski added 2 commits December 12, 2022 10:27
… the same pattern as TargetDB. This also reverts some of the changes to Initialize because they are no longer needed.
@Taztingo Taztingo marked this pull request as ready for review December 12, 2022 15:45
@Taztingo Taztingo requested a review from a team as a code owner December 12, 2022 15:45
Copy link
Contributor

@nullpointer0x00 nullpointer0x00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add change log.

llama-del-rey
llama-del-rey previously approved these changes Dec 12, 2022
@SpicyLemon
Copy link
Contributor

A little background here. I originally wrote it this way (or at least tried) because of the uncertainty around how the db type is configured, and a desire for the core db-migration functionality to be as portable as possible. That is, I tried to write it so that it could convert any databases, not just ones specific to Cosmos. Looking back, there were definitely some flaws in that thinking.

Here's where things can get tricky though. There's two places where the db type can be configured that affect different db directories:

  1. The db_backend config field.
  2. The app_db_backend config field. If this isn't set, it falls back to the db_backend value. This replaced the DBBackend variable that needed to be set during compilation (and didn't fall back or anything).

If I'm remembering correctly, the app_db_backend value defines the application and snapshots dbs, and db_backend defines the rest. Basically, it's possible that not every db is the same type. I wanted the dbmigrator to work on any directory (not specific to data), so I decided it would be best to not hard-code an associate of which db name goes with which configuration parameter. And at the time I started writing it, the dbmigrator didn't have access to that DBBackend variable since it's compiled separately.

Additionally, it's not uncommon to get a data dir, but not the configs associated with it. So I often didn't know what type it was.

arnabmitra
arnabmitra previously approved these changes Dec 14, 2022
arnabmitra
arnabmitra previously approved these changes Dec 14, 2022
Copy link
Contributor

@derekadams derekadams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@SpicyLemon SpicyLemon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Taztingo Taztingo merged commit 6354efc into main Dec 14, 2022
@Taztingo Taztingo deleted the taztingo/1257-update-dbmigrator-to-use-config-toml branch December 14, 2022 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DBMigrator tool cannot convert cleveldb -> goleveldb
7 participants